Thanks for tackling this! Two pieces of feedback:

1. Only throw on type changes

Same-type collisions (dimension ↔ metadata, both strings) overwrite silently but don't corrupt the EMF schema — CloudWatch only rejects when a key flips between string and number. Throwing on same-type clashes is stricter than the spec requires and inconsistent with addDimension, which already handles dimension-on-dimension overwrites with a warn. I'd keep the throw for type changes only and leave same-type collisions on the existing warn path.

2. Centralise the check in serializeMetrics()

The current guards in storeMetric() and addMetadata() only catch two of the four type-change directions. They miss:

• addDimension/addDimensions/setDefaultDimensions after addMetric (num → str)
• addMetric after addMetadata (str → num)

Reverse the call order in either of the existing tests and the corruption comes back. Rather than adding guards to every mutator, serializeMetrics() already has all the sources in scope right before the spread that causes the overwrite — one pass there catches every combination regardless of call order:

// inside serializeMetrics(), just before the return
type Source = 'dimension' | 'default dimension' | 'dimension set' | 'metadata';
const stringKeys = new Map<string, Source>();
for (const k of Object.keys(defaultDimensions)) stringKeys.set(k, 'default dimension');
for (const k of Object.keys(dimensions)) stringKeys.set(k, 'dimension');
for (const set of dimensionSets) for (const k of Object.keys(set)) stringKeys.set(k, 'dimension set');
for (const k of Object.keys(this.#metadataStore.getAll())) stringKeys.set(k, 'metadata');

for (const name of Object.keys(metricValues)) {
  const source = stringKeys.get(name);
  if (source) {
    throw new Error(
      `EMF key collision on "${name}": registered as both a metric (number) and a ${source} (string)`
    );
  }
}

Hi @Zelys-DFKH, do you need any help with this PR? Anything you need me to clarify from my comment?

Sorry for the slow response, I missed the notification. Both points were right.

Pushed a second commit that removes the guards from storeMetric() and addMetadata() and moves the check into serializeMetrics(), where all four sources (default dimensions, dimensions, dimension sets, metadata) are in scope before the spread. The throw is scoped to number-vs-string only, so same-type string overwrites stay on the existing warn path. Tests cover both call orderings for the metadata case and the reverse-order dimension case.

Thanks for the ping.

No worries! Couple of comments, the way the code is now, we only warn on dimesnions overwriting each other but we should warn whenever any keys are overwritten. Secondly, there are merge conflicts in the test file that need to be resolved. Also, the serializeMetrics method has become quite long, you can see there is now a warning about the cyclomatic complexity so we may want to pull this new validation logic into a private method.

Thanks for the thorough review — pushed b2a61fa.

1. setKey now warns whenever any source overwrites another (dimension over default dimension, dimension set over dimension or default dimension), not just when metadata overlaps. The metric throw is unchanged.

2. Synced dimensions.test.ts with upstream's fix(metrics): enforce dimension limits per-array #5229 (loop bounds, toThrow vs toThrowError, the three new upstream tests) and added regression tests for the new warn paths.

3. The repeated get/check/set blocks are now a single setKey helper, which absorbed warnOverwrite. serializeMetrics is unchanged. The complexity was all in #checkKeyCollisions, which is now shorter.

Let me know if anything else needs a look!

Left some comments, also looks like we still have merge conflicts

Thanks for the careful review @svozza. Pushed 3e92ae8 with all three points addressed.

1. Moved EmfKeySource into packages/metrics/src/types/Metrics.ts, re-exported from types/index.ts and imported in Metrics.ts.

2. Dimensions take precedence over metadata. While I was in there, I noticed the spread order in serializeMetrics had metadata last, which was silently overriding dimensions in the output even though the warning said the dimension would win. Flipped the spread (metadata first, dimensions last) and reordered the seed in #checkKeyCollisions to match. Warning message and payload now agree.

3. Cleared the merge conflict by moving my collision tests into a new packages/metrics/tests/unit/keyCollisions.test.ts. dimensions.test.ts is now byte-identical to upstream/main, so GitHub's 3-way merge resolves cleanly. Also dropped DimensionsStore.getDimensionCount() since fix(metrics): enforce dimension limits per-array #5229 stopped using it.

All checks green.

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud